-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: drop user to last chat after leaving a room #31097
Feat: drop user to last chat after leaving a room #31097
Conversation
In small screen, if we want to just return to the home page, we can let method |
Thanks for the updates @eh2077, the Perf. tests are failing i wonder why, can you check? |
I noticed this strange behavior. Screen.Recording.2023-11-10.at.12.20.46.PM.online-video-cutter.com.mp4 |
@getusha Interesting finding. I think it's because we broadcast event when leaving a room, see App/src/libs/actions/Report.js Line 2035 in 358e4b1
and navigate to concierge page here App/src/pages/home/ReportScreen.js Line 322 in 358e4b1
Not sure if we should try to drop to last chat in this case? cc @techievivek |
Coming from your comment here: #31097 (comment), I agree that we should have the logic here as well that will try to drop the user to the previous chat they were on. |
@eh2077 i think yes we should put them on HOLD. could you tell me how 1st and 2nd relate to this? |
Based on @techievivek 's #31097 (comment), I agree the 1st and 2nd are not related this change. |
@eh2077 QQ: How is the 4th issue related to the changes we are making here? |
@techievivek Because I'm going to change this line App/src/pages/home/ReportScreen.js Line 322 in 55cede1
to const lastAccessedReportID = Report.updateRecentlyAccessedReportID('');
if (lastAccessedReportID) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID));
} else {
Report.navigateToConciergeChat();
} which will change the bug report effect of that issue. |
Commented on the issue, @eh2077 is this ready to be tested again? |
@getusha Thanks. Yeah, I just push new changes. With the new changes, below is the demo for this case #31097 (comment) 0-multi-tab-leave-room.mp4Note: After leaving room, the LHN won't be updated automatically. We need to revisit the room to let the left room disappear from LHN. Same behaviour in production. |
@getusha This is ready for review again, see the comment above ^ |
src/libs/actions/Report.js
Outdated
* @returns {String} Returns the most recent accessed reportID | ||
*/ | ||
function getMostRecentlyAccessedReportID() { | ||
return recentlyAccessedReportIDs.length > 0 ? recentlyAccessedReportIDs[recentlyAccessedReportIDs.length - 1] : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more cleaner if we return all report ids and use the last one on the place where we want to use it.
updateRecentlyAccessedReportID(reportID, true);
const recentlyAccessedReportIDs = getMostRecentlyAccessedReportIDs();
const recentlyAccessedReportID = _.last(recentlyAccessedReportIDs);
and use it like that instead of returning a value from updateRecentlyAccessedReportID
, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'd prefer not returning the reportID list because we don't want the reportID array to be potentially modified by client.
and use it like that instead of returning a value from
updateRecentlyAccessedReportID
, what do you think?
I think let method updateRecentlyAccessedReportID
update the stack and return most recently accessed reportID is clean like the use case in method leaveRoom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not returning the reportID list because we don't want the reportID array to be potentially modified by client.
I am not sure i understood this, could you explain it further?
This kind of feels wrong to me returning the same thing from two functions making it duplicate and very limited, i think this will be more cleaner. what do you think @techievivek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think the current two methods are clean enough. See below 3 use cases
- In method
leaveRoom
, we make a single method call
const lastAccessedReportID = updateRecentlyAccessedReportID(reportID, true);
to remove the reportID from stack and return the most recently accessed reportID.
2. In the event listener, we call
Report.updateRecentlyAccessedReportID(reportID);
to update the reportID in the stack and omit the return value.
3. In ReportScreen
, we don't need to update reportID in the stack. So we just call method
const lastAccessedReportID = Report.getMostRecentlyAccessedReportID();
to read the most recently accessed reportID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, it doesn't make sense to me to have two separate functions returning the same value, and i don'
t expect to have a value to be returned from updateRecentlyAccessedReportID
.
- Update the recently accessed report id stack and get the value
updateRecentlyAccessedReportID(reportID, true);
const recentlyAccessedReportIDs = getMostRecentlyAccessedReportIDs();
const recentlyAccessedReportID = _.last(recentlyAccessedReportIDs);
- Get recently accessed report id without updating the value
const recentlyAccessedReportIDs = getMostRecentlyAccessedReportIDs();
const recentlyAccessedReportID = _.last(recentlyAccessedReportIDs);
I'll let @techievivek weigh #31097 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got your point. I'm also fine to
- let method
updateRecentlyAccessedReportID
not return value - let method
getMostRecentlyAccessedReportID
to return the most recently accessed reportID. I think it makes more sense the hide the logic to extract the reportID inside the method.
What do you think?
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-01-18.at.3.05.24.PM.movAndroid: mWeb ChromeScreen.Recording.2024-01-16.at.6.47.46.PM.moviOS: NativeScreen.Recording.2024-01-16.at.7.03.06.PM.moviOS: mWeb SafariScreen.Recording.2024-01-16.at.6.55.57.PM.movMacOS: Chrome / SafariCase 1 Screen.Recording.2024-01-16.at.5.33.19.PM.movCase 2 Screen.Recording.2024-01-16.at.5.36.33.PM.movCase 3 Screen.Recording.2024-01-16.at.5.38.30.PM.movMacOS: DesktopScreen.Recording.2024-01-18.at.3.13.29.PM.mov |
@getusha Updated to skip empty chats. Kindly have another look, thanks! |
@getusha CI passed and please have another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, all cases work well. @techievivek all yours!
@techievivek Gentle bump! |
65c759c
to
756876a
Compare
Solved conflicts from TS migration and re-tested all use cases. |
src/libs/actions/Report.ts
Outdated
|
||
// We want to filter out the current report, hidden reports and empty chats | ||
const filteredReportsByLastRead = sortedReportsByLastRead.filter( | ||
(report) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this be easier if we just rename this to sortedReport
instead of renaming the above report variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes sortedReport
is definitely better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment about the rename. Also, let's avoid force pushing code as that makes reviews harder and get rid of commit history.
src/libs/actions/Report.ts
Outdated
|
||
// We want to filter out the current report, hidden reports and empty chats | ||
const filteredReportsByLastRead = sortedReportsByLastRead.filter( | ||
(report) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I agree
@getusha all yours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.33-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.33-5 🚀
|
policies: {}, | ||
excludeEmptyChats: true, | ||
doesReportHaveViolations: false, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We missed to consider Selfdm here, which led to this bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was merged before we introduced selfDMs 😄 So I would say we should have handled this in the design doc rather.
Details
Fixed Issues
$ #30778
PROPOSAL: #30778 (comment)
Tests
=== Preparation ===
room-1
,room-2
,room-3
androom-4
=== Case 1 ===
room-1
room-1
=== Case 2 ===
room-2
room-3
#room-3
#room-2
#room-2
=== case 3 (Web only) ===
room-4
room-4
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
0-android.mp4
Android: mWeb Chrome
0-mobile-chrome.mp4
iOS: Native
0-ios.mp4
iOS: mWeb Safari
0-mobile-safari.mp4
MacOS: Chrome / Safari
0-web.mp4
MacOS: Desktop
0-desktop.mp4